Pre-Flight Review — CORE-152486

Branch: CORE-152486 vs master | Scope: 20 files, tiered storage cache PVC shrink feature

Summary

The feature design is sound — CR status as state store, atomicity ordering (state before PVC creation), size-based PVC identification, and treating terminating pods as absent are all correct for a Kubernetes operator. One HIGH bug must be fixed before merging. Eight MEDIUM observations are recommended fixes; six LOW items are optional improvements.

Alignment Check

Findings

HIGH — Fix before merging

C1 — deleteRemovedCachePVCs deletes a mounted PVC
pkg/resources/kafka/kafka.go

When a TieredStorageCache: true storage config is removed while the broker pod is still running and no CacheVolumeStates entry exists for that mount path, handleBrokerCacheResizeCleanup returns early (len(cacheVolumeStates) == 0), then deleteRemovedCachePVCs runs unconditionally and issues a Delete against the actively-mounted PVC. The PVC-protection finalizer prevents immediate data loss, but the delete is issued unintentionally.

cleanupOrphanedDuplicateCachePVCs (correctly) gates on !brokerPodExists. deleteRemovedCachePVCs must do the same:

if !brokerPodExists {
    if err := r.deleteRemovedCachePVCs(ctx, log, brokerId, pvcList, desiredMountPaths); err != nil {
        return err
    }
}

MEDIUM — Strongly recommended

C4 — Missing test: crash re-entry with no replacement PVC
pkg/resources/kafka/kafka_test.go

No test covers: initialCacheVolumeState = CacheResizePendingDeletion, only the old PVC exists (reconciler crashed between writing state and creating replacement), pod running. Expected: no Delete, no state clear, one Create (replacement re-staged). The invariant is described in comments but untested.

A1 — Empty-string sentinel for map deletion is an implicit contract
pkg/k8sutil/status.go:182

CacheResizeState("") silently deletes map entries in generateBrokerState. The established pattern is DeleteVolumeStatus. An uninitialized CacheResizeState variable passed by a future caller would delete entries with no compile error. Consider a named constant (CacheResizeStateNone) or a dedicated DeleteCacheVolumeStatus helper.

A2 — "tieredStorageCache" annotation key duplicated as bare string 9+ times
pkg/resources/kafka/kafka.go, pvc.go, kafka_test.go

A typo at any read site silently treats a cache PVC as a regular data volume. Define a named constant (e.g., TieredStorageCacheAnnotation = "tieredStorageCache") following the BrokerIdLabelKey precedent.

A5 — Tiered-cache PVC guard predicate duplicated at 3 sites
pkg/resources/kafka/kafka.go:1298, 1326, 1339

pvc.DeletionTimestamp != nil || pvc.Annotations["tieredStorageCache"] != annotationTrue appears three times. Extract to a helper (isTieredCachePVC).

SE1 — PVC deletion gated only on mutable annotations, no OwnerReference check
pkg/resources/kafka/kafka.go — deleteRemovedCachePVCs, cleanupOrphanedDuplicateCachePVCs

The List selector (LabelsForKafka + BrokerIdLabelKey) and annotation filter (tieredStorageCache, mountPath) have no OwnerReference verification. A principal with patch access to PVCs in the namespace could craft a matching PVC to trigger deletion. Add an ownership guard before deleting in both helpers.

SE2 — Annotation backfill enables two-step privilege escalation
pkg/resources/kafka/kafka.go:1608

A KafkaCluster CR writer (without direct PVC delete access) can: (1) set TieredStorageCache: true on an existing data volume's storage config — operator backfills the annotation; (2) remove the storage config entry — deleteRemovedCachePVCs deletes the now-annotated data PVC, bypassing the CC disk-removal workflow. Consider restricting backfill to ClaimPending PVCs or adding a validation webhook.

D1 — Rollback mid-resize leaves two PVCs indefinitely with no recovery path
docs/tiered-storage-pvc-resize.md

If the operator is rolled back while pending-deletion state is set, the old operator ignores the unknown field — two PVCs coexist at the same mount path with no automatic recovery and no visible signal in kubectl describe kafkacluster. Add a "Rollback safety" section with detection steps and a manual recovery procedure.

D2 — No Kubernetes Events for resize lifecycle
pkg/resources/kafka/kafka.go

The entire resize lifecycle is visible only in operator logs. Emit record.Event(...) at minimum when pending-deletion is first written and when cleanup completes. This is the standard on-call triage interface; without it, a broker restart from a resize is indistinguishable from a crash without operator log access.

LOW — Optional improvements

Rejected Findings